Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

get_eth1_data uses timestamp instead of block height #1553

Merged
merged 1 commit into from
Jan 5, 2020
Merged

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented Jan 3, 2020

Address #1537

@djrtwo
Copy link
Contributor Author

djrtwo commented Jan 3, 2020

cc @paulhauner

@djrtwo djrtwo changed the title modify get_eth1_data to use timestamp instead of block height get_eth1_data uses timestamp instead of block height Jan 3, 2020
Copy link
Collaborator

@protolambda protolambda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +272 to +273
block.timestamp <= period_start - SECONDS_PER_ETH1_BLOCK * ETH1_FOLLOW_DISTANCE
and block.timestamp >= period_start - SECONDS_PER_ETH1_BLOCK * ETH1_FOLLOW_DISTANCE * 2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the simplicity, but aside from a deep re-org, but just leaving a note here that a very slow (deprecation) or dead (with a remaining tail of deposits) eth1 will also be an extra-protocol thing for eth2.

```python
def voting_period_start_time(state: BeaconState) -> uint64:
eth1_voting_period_start_slot = state.slot % SLOTS_PER_ETH1_VOTING_PERIOD
return state.genesis_time + eth1_voting_period_start_slot * SECONDS_PER_SLOT
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the slot/epoch conversions, helper functions for time - slot conversion would be useful too. But leaving this as-is, we can change it when there are more cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants